Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more details on _y #482

Closed
wants to merge 1 commit into from

Conversation

Arthur-Milchior
Copy link
Contributor

I found

Let's inspect those stack/local variables

confusing. Indeed, the sentence mentions multiple variables, but a single one was inspected. It was even more surprising because when I entered print _y, I got the result 42. (I later understood that it is because the code was run twice, and the value assigned on first run)

For the sake of the consistency, it seems also interesting to mention what the learner would get if they were trying to read _y, its address, and maybe also why they could get the correct answer while the value is unitialized.

I considered mentioning that this statement could be checked by switching from 42 to any other number just before running. This would ensure _y can not be initialized.

@rust-highfive
Copy link

r? @andre-richter

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources labels Nov 12, 2022
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I found the additional explanation about _y before its initialization rather confusing.
If the concern is that only x is inspected and the title says "let's inspect those" I would rather change that title.
However, I do not consider it confusing because the _y variable is also inspected just a few lines below.

@Arthur-Milchior
Copy link
Contributor Author

The sentence I quoted was not a title. It was the sentence introducing the code excerpt. So I assumed that it was describing what the code was doing. It does not seems to refer to the whole section.
I'd be fine changing this sentence.

But, as a reader, what confused me most is something slightly more complex.
You wrote that _y is not yet assigned. Since I had gdb open while reading this, I found natural to check what would then occur if I just tried to print _y's value. And I got very confused, because I got 42. It could be a coincidence, but it would be a strange one. Especially since I could repeat the experiment and still got 42. I know that, when I use GDB on desktop, I would usually get random value for unitialized variables.
My first hypothesis was that the book was wrong for some reason. Maybe instead of line 13, it should have been break at line 12. Or maybe something changed since the book was written.

So, I assumed that if something confused me, I may not be the only one who gets confused by it, and wanted to help the next reader who may get lost at the same point.

Admittedly, I almost never run GDB over such code. Usually, I run it over real code I want to debug, and where it's guaranteed that the position in the stack was overwritten many time between two run. And anyway, I would not be guaranteed that the OS give the same physical address in memory to two different runs. So it seemed interesting too to note that there is a difference between embedded software and software running on top of OS.

However, while I believe that this information is potentially interesting for future reader, I don't claim my wording is the best one ,and I'd be very happy to see it improved. I hesitated between creating an issue or directly a PR, and assumed a PR would be simpler to show exactly what extra information I'd find useful, even if it would not be merged as-is

@eldruin
Copy link
Member

eldruin commented Nov 14, 2022

I understand. Thank you for the explanation.
On balance, this is an introductory book to microcontrollers so the target audience are rather beginners.
The section concerning this PR is potentially the very first program a user flashed onto their device and even the first time they fire up gdb. Certainly is the first program in the book.
This is why I think the additional explanation about uninitialized-or-not memory and multiple runs rather adds confusion to a mind where a lot is already going on.
Whether this explanation would be fine in a different form, I do not know. As it stands. I would rather not include it.
As I said, I would be fine with some rewording of the introductory text if deemed unclear.
Any other opinions @rust-embedded/resources?

Copy link
Member

@BartMassey BartMassey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, the confusing part of the existing documentation is the claim that _y is not initialized. This may not be true in practice: the compiler may go ahead and initialize it.

That's not what seems to be happening here, though: the assembly seems to show that the 42 is indeed a coincidence.

        movl    $42, -8(%rsp)                                                   
.Ltmp24:                                                                        
        .loc    8 3 13 is_stmt 0                                                
        movl    $42, -4(%rsp)                                                   

All in all, I think it would be better to just reword this to avoid the whole issue of when things get initialized. It's a confusing topic that's a little out of scope at this stage of the game, I think.

Moving from plural to singular is a better description of what is
actually occurring. After all, only `x` is considered in the example
of use of gdb.

As mentioned by @BartMassey in rust-embedded#482, it is possible that `y` is
initialized. Given that discussing whether something is really
initialized or not is out of scope of this book, or at least chapter,
this part of the sentence is deleted and not replaced.
@Arthur-Milchior
Copy link
Contributor Author

Sorry, missed those answers. I edited the commit, and just removed the few part of the sentences that were not accurate.

I'll be honest @BartMassey, I don't know assembly at all, as much as I'd love to take the time to discover this world. So I've no idea what the assembly you showed us mean.
So I fail to understand how this coincidence works

@BartMassey
Copy link
Member

Thanks for raising this again! That said, I'm currently editing https://github.com/rust-embedded/discovery-mb2 as an updated version of this book: if you wanted to look at what's there I'll go ahead with cleaning it up some if you still have concerns.

@Arthur-Milchior
Copy link
Contributor Author

You're welcome. From my point of view, I just want over my thousands (sic) of github notification, prioritizing the PR I opened.

I fear I won't have time to read a full book again. I'm not even sure where I put my microcontroller. If there is any specific file corresponding to the one I correcetd, I can take a look, but I won't do more in the foreseeable future.

If this PR if of no interest, feel free to close it, I won't be offended

@BartMassey
Copy link
Member

Thanks for your input. I've made some corrections in the new version of the book based on your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants